Thanks @CattleProdigy, this is great. Hopefully we can soon land a nice serde impl by combining your effort and @w1th0utnam3's work in #1019.

I just took a brief look, and it's quite elegant. I'm a little confused how it actually works, though, maybe you can help me understand? For example, here (see comment):

 impl<'de, T: Deserialize<'de>> Deserialize<'de> for CsMatrix<T> {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        // Why doesn't this lead to infinite recursion?
        // Doesn't this call the same method as we're currently implementing?
        let unchecked = Self::deserialize(deserializer)?;

OK, so having looked at the two PRs, I think it would make sense to go with @w1th0utnam3's general approach: Although I think the output representation is not all that important, we actually do sometimes dump data to e.g. JSON, in which case it would be useful to have a more natural representation of the data, as it's easier to read and consequently debug. I also feel a little bit queasy about about constructing an "invalid" CSR/CSC matrix (even temporarily) with the remote approach - but this is perhaps unfounded and ultimately not a decision factor.

However, it would be nice to re-use the effort you have taken in writing tests. Perhaps we could port over the test cases @w1th0utnam3 ? @CattleProdigy: if you want to retain the ownership to those tests (by way of entries in the commit log), we can add those tests in a separate PR after merging #1019. What do you think?

I'm back from vacation.

Thanks @CattleProdigy, this is great. Hopefully we can soon land a nice serde impl by combining your effort and @w1th0utnam3's work in #1019.

I just took a brief look, and it's quite elegant. I'm a little confused how it actually works, though, maybe you can help me understand? For example, here (see comment):

 impl<'de, T: Deserialize<'de>> Deserialize<'de> for CsMatrix<T> {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        // Why doesn't this lead to infinite recursion?
        // Doesn't this call the same method as we're currently implementing?
        let unchecked = Self::deserialize(deserializer)?;

I don't actually know the answer to this just yet, but since we're not going with this approach. It'll remain a mystery for now.

OK, so having looked at the two PRs, I think it would make sense to go with @w1th0utnam3's general approach: Although I think the output representation is not all that important, we actually do sometimes dump data to e.g. JSON, in which case it would be useful to have a more natural representation of the data, as it's easier to read and consequently debug. I also feel a little bit queasy about about constructing an "invalid" CSR/CSC matrix (even temporarily) with the remote approach - but this is perhaps unfounded and ultimately not a decision factor.

However, it would be nice to re-use the effort you have taken in writing tests. Perhaps we could port over the test cases @w1th0utnam3 ? @CattleProdigy: if you want to retain the ownership to those tests (by way of entries in the commit log), we can add those tests in a separate PR after merging #1019. What do you think?

That's fine by me. I don't share your concern about invalid construction, but the remote approach is exotic enough that perhaps it's worth avoiding. It doesn't save that much code anyways.

R/e tests: I'm curious what aspects of my testing we'd like to retain. I felt my tests were a bit half-assed because I was trying to get the PR up in time for discussion. I'll look over @w1th0utnam3 's branch and see where more testing might be helpful. We have a few options logistically:

1. Separate PR after Implement Serde Serialize/Deserialize for nalgebra-sparse types #1019
2. I open a PR with more tests against @w1th0utnam3 's branch. He merges that and then we merge Implement Serde Serialize/Deserialize for nalgebra-sparse types #1019.
3. I push commits with tests to a branch of my own and @w1th0utnam3 cherry picks them. In that case, he'd be listed as committer and I'd be the commit author on those commits, which is fine by me.

I'd vote for (2) over (1) since it provides just one PR which closes out #876 and provides a single point for people to look back on. I think (3) is a worse version of (2).

@CattleProdigy: I agree that option (2) makes sense. @w1th0utnam3: any thoughts?

Yeah, (2) sounds good to me 👍

I haven't had time to hack on this further so I'm closing. I don't have anything to contribute to the other branch right now.

@w1th0utnam3 if there's something specific you need help with, just let me know.

@CattleProdigy: appreciate all your input so far! We'll go ahead and get the other branch merged, so that we all can use serialization with nalgebra-sparse 👏